This PR was auto-closed. Only contributors approved with lgtm can open PRs. Open an issue first.

Maintainers review auto-closed issues and reopen worthwhile ones. Issues that do not meet the quality bar in CONTRIBUTING.md may not be reopened or receive a reply.

If a maintainer replies lgtmi, your future issues will stay open. If a maintainer replies lgtm, your future issues and PRs will stay open.

See CONTRIBUTING.md.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	cargo/​filetime@​0.2.29

View full report

Thanks for the careful writeup — the motivation is solid and the SQL path is well reasoned. One correctness concern and a couple of robustness/nit items before this is safe to merge.

Context

The authoritative date filter is filter_loaded_entries_by_date (adapter/claude/mod.rs:121), which runs after load and filters on each entry's logical date (time.created converted to TZ). So the new loader filtering is purely a load-time optimization, and the only correctness requirement is: the loader must never drop an entry that filter_loaded_entries_by_date would keep. Dropping extra rows just makes it slower; dropping kept rows silently under-counts.

✅ SQL WHERE time_created — looks correct

The -1 day / +2 day slack absorbs TZ offset (max ±14h < 1 day) and any small drift between the time_created column and JSON data.time.created, so it's always wider than the logical-date filter. No risk of dropping kept rows, and it rides the existing index. 👍

⚠️ JSON mtime skip on the --until side — silent under-count risk

File mtime is not guaranteed to match the message's logical time. cp -r, rsync without -a, backup restore, and cloud sync all reset mtime to "now". That's exactly the "legacy pre-SQLite dump migrated to a new machine" scenario this PR targets.

• --since (skip when modified_ms < min): if mtime is reset newer than real, the file is not skipped → no loss. Reasonably safe.
• --until (skip when modified_ms >= max): if a legacy file's mtime is reset to "now" but its logical date is in the past, an --until <past date> query will skip the file even though its logical date is in range → silent under-count.

"The summary filter is authoritative" does not protect this path, because the skip happens before load, so dropped rows never reach the authoritative filter. The new tests set mtime explicitly and so don't exercise the mtime-vs-logical-time divergence.

Suggested options:

• Drop the --until mtime skip (the --since skip alone should capture most of the 186s → 9.57s win; legacy data is old and rarely hits the upper bound anyway), or
• Make the mtime skip explicitly opt-in/heuristic and document that --until may under-count on environments where files were copied without preserving timestamps.

⚠️ DB schema without time_created — silent empty DB results

If an older/variant OpenCode schema lacks time_created, connection.prepare(sql) fails and the else branch returns Vec::new(). That means DB entries silently disappear only when --since/--until is set, while the no-filter path still works — an asymmetric regression. Consider falling back to the unfiltered query on prepare failure.

Nits

• The JSON loop calls fs::metadata(&file) twice per file (once for since, once for until). With 118k files that's a lot of extra stats — fold into a single metadata call.
• The two mtime blocks are near-duplicates; a small helper would read better.
• filetime as a dev-dependency only is fine.

Net: SQL path is good to go; the --until mtime skip and the prepare-failure fallback are the two things I'd want addressed first.

@justi heads up — #1220 (now closed) tackled the same issue with a different JSON-side strategy that I think is worth pulling into this PR.

Instead of skipping JSON files by file mtime, it extracts the real time.created millis straight from the raw JSON string and runs the exact same date check the authoritative filter_loaded_entries_by_date uses (format_date_tz → compare against since/until). That sidesteps the mtime concern I raised above entirely: it's exact, needs no -1/+2 day slack, and if extraction fails it falls through to a full parse (fails open), so it can never drop a row the authoritative filter would keep.

Your SQLite WHERE time_created push-down is the better half — it's the part that actually uses the index and gives the big speedup, which #1220 lacked. So the ideal is the combination:

• SQLite: keep your indexed WHERE time_created predicate (the real perf win).
• JSON files: replace the mtime skip with feat(opencode): pre-filter messages by --since/--until in loader #1220's content-based time.created extraction + the shared date check.

That keeps the speedup while removing the silent under-count risk. Closing #1220 so this stays the single canonical PR — would you be up for folding that JSON approach in here? Happy to point you at the exact extract_message_timestamp / timestamp_within_range helpers from that PR.

Following up on your suggestion to fold #1220's JSON strategy into this PR.

I've combined both halves as you outlined:

• SQLite: kept the indexed WHERE time_created push-down with the -1d/+2d slack (the real index speedup).
• JSON files: replaced the mtime skip with feat(opencode): pre-filter messages by --since/--until in loader #1220's content-based time.created extraction, running the same format_date_tz check as the authoritative filter_loaded_entries_by_date and failing open to a full parse. This removes the mtime under-count risk you raised.

I also addressed the prepare-failure fallback: a schema without time_created now falls back to an unfiltered scan (the in-loop date check still applies) instead of returning no rows.

Verified on a real 33 GB / 395k-row opencode.db: the time_created column equals the payload time.created in all 395,289 rows (max delta 0 ms), so the push-down can't drop a row the authoritative filter would keep. A one-month --since/--until window returns correct totals; the full unit suite, clippy, and fmt are clean.

Would you be willing to reopen this (or drop an lgtm) so I can push the combined version?